Skip to content

Conversation

@zeonchew
Copy link
Contributor

@zeonchew zeonchew commented Oct 8, 2024

This PR adds support for UDC for Ambiq Apollo4p family. The boards tested are:

  • apollo4p_evb
  • apollo4p_blue_kxr_evb

Followings are the USB samples tested:

  • cdc_acm
  • console
  • hid-keyboard
  • hid-mouse
  • mass

@zephyrbot
Copy link

zephyrbot commented Oct 8, 2024

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Later the code sets the iso capability on the endpoints. Are the endpoints limited in hardware to these specific values and not the maximum allowed values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tmon-nordic, there is a limitation that the end of transaction is not able to be identified when OUT endpoint received data that is multiple of endpoint MPS, when the data count is lower than the triggered size. Hence, the safest way to expose this now is to limit it to 64 bytes for FS and 512 bytes for HS.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this limitation, in your shim driver, HAL driver, or controller?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be categorized as controller limitation.

@zeonchew zeonchew force-pushed the apollo4p-usb-udc branch 2 times, most recently from 9290c51 to a13fc8c Compare October 8, 2024 10:41
@zeonchew zeonchew force-pushed the apollo4p-usb-udc branch 2 times, most recently from b8f2d43 to 7c3fdab Compare October 9, 2024 10:36
aaronyegx
aaronyegx previously approved these changes Oct 15, 2024
Copy link
Contributor

@aaronyegx aaronyegx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The boards and dts changes look good to me.

@zeonchew
Copy link
Contributor Author

Hi @jfischer-no, I believe your comments above are addressed, please help to have a review again. Thank you 😉

tmon-nordic
tmon-nordic previously approved these changes Oct 18, 2024
AlessandroLuo
AlessandroLuo previously approved these changes Oct 21, 2024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this limitation, in your shim driver, HAL driver, or controller?

Added user button for apollo4p_blue_kxr_evb and apollo4p_evb

Signed-off-by: Chew Zeh Yang <[email protected]>
tmon-nordic
tmon-nordic previously approved these changes Oct 24, 2024
Added ambiq-usb bindings needed by udc_ambiq.

Signed-off-by: Chew Zeh Yang <[email protected]>
Comment on lines 68 to 69
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, there is no need to prefix it with usb_.

	struct gpio_dt_spec vddusb33_gpio;
	struct gpio_dt_spec vddusb0p9_gpio;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jfischer-no, I think I have missed that point earlier. removed usb_ from the 2 variable names

Comment on lines +158 to +160
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

		am_hal_usb_intr_usb_enable(priv->usb_handle,
					   USB_CFG2_SOFE_Msk | USB_CFG2_ResumeE_Msk |
					   USB_CFG2_SuspendE_Msk | USB_CFG2_ResetE_Msk);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-format keeps add a tab there. I have manually reverted the change by clang-format and push. Let's see whether the CI will let us through 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value is never checked, maybe static void usb_power_rails_set(const struct device *dev, bool on)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jfischer-no, since the error status return might be useful to indicate that the power rail control is not successful, I have made the change to propagate the error to upper layer. Please help to have a look at the change and see whether it is okay.

Comment on lines 486 to 494
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just on, bool is either 0 or 1 and can be promoted to any integer type.

	/* power rails set */
	ret = gpio_pin_set_dt(&cfg->usb_vddusb33_gpio, on);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jfischer-no, understood and made the change. Will use implicit bool to integer casting in the future.

Added implementation for udc_ambiq with its required Kconfig and
CMakelist updates.

Signed-off-by: Chew Zeh Yang <[email protected]>
Add USB node to apollo4p and apollo4p_blue qualifier, and apollo4p_evb
and apollo4p_blue_kxr_evb board to enableUSB support on the MCU and
its EVB.

Signed-off-by: Chew Zeh Yang <[email protected]>
@jfischer-no jfischer-no added this to the v4.1.0 milestone Oct 28, 2024
@nashif nashif merged commit 0facdd8 into zephyrproject-rtos:main Nov 16, 2024
26 checks passed
@zeonchew zeonchew deleted the apollo4p-usb-udc branch July 29, 2025 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Samples Samples area: USB Universal Serial Bus platform: Ambiq Ambiq

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants